Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronous producer - Handle acknowledgements of sent PUB/MPUB/DPUB messages #52

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Soulou
Copy link
Contributor

@Soulou Soulou commented Jan 17, 2019

The goal of this PR is to update the library to ensure that an exception is raised if an error happens when a message is sent to NSQd.

This is a work in progress, all the inputs are welcomed.

Related #51

@Soulou
Copy link
Contributor Author

Soulou commented Jan 17, 2019

The first step has been to gather read/write operations in a single thread in order to share information between both operations in an easier way.

@Soulou
Copy link
Contributor Author

Soulou commented Jan 17, 2019

Second step, not sure it's the best implementation but a SizableQueue of 1 item is created for each message and publishing Thread is waiting for the response to be received. I got inspired from the go implementation which is handling it this way.

@Soulou
Copy link
Contributor Author

Soulou commented Jan 18, 2019

I think the PR is ready, the API hasn't changed, the behavior either, but the internals have slightly been updated as there is only one read/write loop based on a select.

The code is ready for review. All the old specs are passing correctly.

@Soulou Soulou changed the title WiP - Handle message acknowledgement of sent messages Synchronous producer - Handle acknowledgements of sent PUB/MPUB messages Jan 18, 2019
@Soulou
Copy link
Contributor Author

Soulou commented Jan 21, 2019

@bschwartz a small follow up to get your input on this.

This commit has been inspired greatly by what is done in the official
golang driver.

To follow the recommandations of the NSQ team, the producer is not able
to get initialized with nsqlookupd URL, a producer is connecting to
only one NSQd instance, not several.

- The producer is keeping a list of transactions to wait for the return
value of NSQd.
- A new class `NsqdsProducer` can be initialized with multiple NSQds
addresses and will apply a strategy to write messages:
  - `Nsq::NsqdsProducer::STRATEGY_FAILOVER` Always send on the same
nsqd except if not available, then get to the next one
  - `Nsq::NsqdsProducer::STRATEGY_ROUND_ROBIN` Apply round robin over
the different available nsqds
@Soulou
Copy link
Contributor Author

Soulou commented Jan 29, 2019

There is a race condition on the failing tests.

if SUB is sent too early in nsq starting process, it is refused with E_INVALID invalid SUB during init state
As it's not detected, the consumer is not registering on the topic and not getting the message.

The bug exists on master too, all the other specs are passing

@bschwartz
Copy link
Member

This is looking good @Soulou. I should have time Friday for a more in depth review. In the meantime, do you think you could also update the README to explain the new functionality?

Also, if you have some ideas of how to fix the spec issue that you mentioned also exists on master, that would be great!

@Soulou
Copy link
Contributor Author

Soulou commented Jan 30, 2019

Thanks @bschwartz I'll update the README and try to fix the spec,

@Soulou Soulou changed the title Synchronous producer - Handle acknowledgements of sent PUB/MPUB messages Synchronous producer - Handle acknowledgements of sent PUB/MPUB/DPUB messages Jan 30, 2019
@Soulou
Copy link
Contributor Author

Soulou commented Jan 30, 2019

For info, this PR is already running in our production environment, so far so good.

@Soulou
Copy link
Contributor Author

Soulou commented Jan 31, 2019

@bschwartz done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants